Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jul 9, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Added new tests for invalid variant metadata and value.

Are these changes tested?

Yes, added new tests.

Are there any user-facing changes?

No

@github-actions github-actions bot added the parquet Changes to the parquet crate label Jul 9, 2025
@viirya viirya force-pushed the add_invalid_object_test branch from c328d89 to 1a3ac22 Compare July 9, 2025 07:50
@viirya viirya force-pushed the add_invalid_object_test branch from 1a3ac22 to 3878891 Compare July 9, 2025 07:53
@viirya viirya requested a review from alamb July 9, 2025 08:00
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- thanks @viirya

cc @scovich @superserious-dev @friendlymatthew

Copy link
Contributor

@friendlymatthew friendlymatthew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thank you @viirya

Comment on lines +544 to +545
assert!(err.is_err());
let err = err.unwrap_err();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: assert! and unwrap_err will both panic if the result is not an error. Seems redundant?
(above as well)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's maybe clean it up in a follow on PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you meant to remove assert!(err.is_err());? Yea, let's remove it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, maybe we can make a follow on PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The follow on pr #7897

@alamb
Copy link
Contributor

alamb commented Jul 10, 2025

Thanks everyone!

@alamb alamb merged commit ff3a2f2 into apache:main Jul 10, 2025
12 checks passed
@viirya
Copy link
Member Author

viirya commented Jul 10, 2025

Thanks @alamb @friendlymatthew @mbrobbel @scovich

alamb added a commit that referenced this pull request Jul 11, 2025
# Which issue does this PR close?

None

# Rationale for this change

Address the comment
#7885 (comment).

# What changes are included in this PR?

Remove redundant `is_err` checks.

# Are these changes tested?

Existing tests.

# Are there any user-facing changes?

None

Co-authored-by: Liang-Chi Hsieh <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Variant] Add tests for invalid variant values (aka verify invalid inputs)

5 participants